Skip to content

refactor logger to simple logger#425

Open
kfirstri wants to merge 11 commits intomainfrom
refactor-logger
Open

refactor logger to simple logger#425
kfirstri wants to merge 11 commits intomainfrom
refactor-logger

Conversation

@kfirstri
Copy link
Collaborator

@kfirstri kfirstri commented Mar 17, 2026

Note

Description

This PR extracts a @base44-cli/logger package that provides a Logger interface with two implementations: ClackLogger for interactive TTY sessions (delegates to @clack/prompts) and SimpleLogger for non-interactive/CI environments (writes plain text to stderr). The logger is injected into CLIContext and passed as the first argument to all action functions via Base44Command, replacing direct @clack/prompts log imports throughout commands.

Related Issue

None

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Other (please describe):

Changes Made

  • Created packages/logger workspace package (@base44-cli/logger) with a Logger interface, ClackLogger (interactive), and SimpleLogger (CI/non-interactive)
  • Added log: Logger to CLIContext — instantiated as ClackLogger or SimpleLogger based on TTY detection at startup in runCLI()
  • Updated Base44Command.action() to inject CLIContext as the first argument to every action function, removing the need for commands to access it via the Commander command instance
  • Removed the isNonInteractive getter from Base44Command (now accessed via destructured context)
  • Updated ensureAuth and ensureAppConfig middleware to accept CLIContext instead of just ErrorReporter
  • Migrated all command files to destructure { log } / { log, isNonInteractive } from the injected context instead of importing log from @clack/prompts directly
  • Updated docs/commands.md to reflect the new context-injection pattern and Logger usage
  • Re-exported Logger, ClackLogger, and SimpleLogger from @/cli/utils/index.js

Testing

  • I have tested these changes locally
  • I have added/updated tests as needed
  • All tests pass (npm test)

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • I have updated docs/ (AGENTS.md) if I made architectural changes

Additional Notes

The Logger interface maps 1:1 to @clack/prompts log methods (info, success, warn, error, step, message), so migrating commands is mechanical. The SimpleLogger writes all output to stderr to keep stdout clean for machine-readable output in CI pipelines.


🤖 Generated by Claude | 2026-03-23 00:00 UTC

@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2026

🚀 Package Preview Available!


Install this PR's preview build with npm:

npm i @base44-preview/cli@0.0.47-pr.425.4dfac63

Prefer not to change any import paths? Install using npm alias so your code still imports base44:

npm i "base44@npm:@base44-preview/cli@0.0.47-pr.425.4dfac63"

Or add it to your package.json dependencies:

{
  "dependencies": {
    "base44": "npm:@base44-preview/cli@0.0.47-pr.425.4dfac63"
  }
}

Preview published to npm registry — try new features instantly!

@kfirstri kfirstri moved this from Backlog to In progress in CLI Development Mar 18, 2026
@kfirstri kfirstri requested a review from artemdemo March 19, 2026 13:52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe logger should be a separate package?
wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this idea, what do you think about the name @base44-cli/logger? maybe just @base44/logger?

"version": "0.0.1",
"description": "Logger interface and implementations for Base44 CLI",
"type": "module",
"main": "./src/index.ts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need main?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so our tsconfig was set to "moduleResolution": "node" which didn't support "exports", only "main".. working on this

"ignoreDependencies": ["@types/deno"]
},
"packages/logger": {
"project": ["src/**/*.ts"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you don't need entry?
Why it's needed in packages/cli?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looked it up, knip recognize the entry by the exports, and since the cli package uses the bin/ folder we need to specify it.. but i removed it for logger package.

knip.json Outdated
@@ -7,6 +7,9 @@
"project": ["src/**/*.ts", "tests/**/*.ts"],
"ignore": ["dist/**", "tests/fixtures/**"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need "dist/**" in ignore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, removed

# Conflicts:
#	bun.lock
#	packages/cli/src/cli/utils/command/Base44Command.ts
@kfirstri kfirstri requested a review from artemdemo March 23, 2026 16:56
"module": "ES2022",
"lib": ["ES2022"],
"moduleResolution": "node",
"moduleResolution": "bundler",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artemdemo what do you think about this?
this makes it so packag.json will support "exports" and not just "main"
in wix-cli we use "node16", but it seems it raises other issues.. maybe i can first bump to node16 in another branch and then fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants